Portability cleanup setsockopt() calls #1359

yadij · 2023-05-20T12:51:22Z

error: cannot convert 'int*' to 'const char*'

Ensure that parameter 4 is cast properly to char* which is
known to be compatible with all known API declarations
for this function.

Ensure that parameter 5 is passed the size of object in
parameter 4 whenever possible, instead of the theoretical
type size.

src/ip/QosConfig.cc

#1359 (comment)

src/ip/QosConfig.cc

rousskov · 2023-05-24T20:27:06Z

src/WinSvc.cc

@@ -965,7 +965,7 @@ static int Win32SockInit(void)
        } else {
            opt = opt | SO_SYNCHRONOUS_NONALERT;

-            if (::setsockopt(INVALID_SOCKET, SOL_SOCKET, SO_OPENTYPE, (char *) &opt, optlen)) {
+            if (::setsockopt(INVALID_SOCKET, SOL_SOCKET, SO_OPENTYPE, reinterpret_cast<char *>(&opt), optlen)) {


If we are going to change so many callers, like the current PR iteration does, then we should polish/generalize and make our SetSocketOption() available to this and similar code, allowing us to move all the current ugly and poorly duplicated manual casts into one place.

The same approach will correctly handle the last/size arguments with less noise and fewer opportunities for bugs.

FWIW, I am happy to help with generalizing/polishing the existing SetSocketOption() code.

As demonstrated in PR #1373 this is a deceptive request. Acceding will result in a fall down the slippery slope of ever increasing perfection requests. Throwing this PR further out of the intended scope of fixing a few MinGW compile errors.

As demonstrated in PR #1373 this is a deceptive request. Acceding will result in a fall down the slippery slope of ever increasing perfection requests. Throwing this PR further out of the intended scope of fixing a few MinGW compile errors.

I disagree with the above assessment and speculation. However, may not need to agree on this to make progress: If you refactor this PR so that it is just "fixing a few MinGW compile errors", without modifying so many setsockopt() calls (most of which are not specific and some are probably not even applicable to MinGW builds), then this change request will become stale/inapplicable and can be resolved on those grounds. I do not know whether (and am not implying that) such refactoring is possible.

AFAICT, this change request has not been addressed when PR branch was force-pushed from 02b82a1 to 213df95 and my re-review requested a few hours ago.

It was addressed by authors decision not to implement your requested change.

AFAICT, this change request has not been addressed when PR branch was force-pushed from 02b82a1 to 213df95 and my re-review requested a few hours ago.

It was addressed by authors decision not to implement your requested change.

Thank you for disclosing that decision; I was not aware of that decision when my re-review was requested.

This change request stands. Doing nothing does not address my blocking concern.

yadij · 2023-06-03T17:33:47Z

Waiting on PR #1373

yadij · 2023-06-04T17:26:47Z

No longer going to wait for PR #1373 which has already started devolving into requests for perfection.

rousskov · 2023-06-04T17:36:23Z

src/WinSvc.cc

@@ -965,7 +965,7 @@ static int Win32SockInit(void)
        } else {
            opt = opt | SO_SYNCHRONOUS_NONALERT;

-            if (::setsockopt(INVALID_SOCKET, SOL_SOCKET, SO_OPENTYPE, (char *) &opt, optlen)) {
+            if (::setsockopt(INVALID_SOCKET, SOL_SOCKET, SO_OPENTYPE, reinterpret_cast<char *>(&opt), optlen)) {


As demonstrated in PR #1373 this is a deceptive request. Acceding will result in a fall down the slippery slope of ever increasing perfection requests. Throwing this PR further out of the intended scope of fixing a few MinGW compile errors.

I disagree with the above assessment and speculation. However, may not need to agree on this to make progress: If you refactor this PR so that it is just "fixing a few MinGW compile errors", without modifying so many setsockopt() calls (most of which are not specific and some are probably not even applicable to MinGW builds), then this change request will become stale/inapplicable and can be resolved on those grounds. I do not know whether (and am not implying that) such refactoring is possible.

Ensure that parameter 4 is cast properly to char* which is known to be compatible with all known API declarations for this function. Ensure that parameter 5 is passed the size of object in parameter 4 whenever possible, instead of the theoretical type size.

rousskov

Please address the earlier change request at #1359 (comment)

yadij · 2024-11-21T00:12:03Z

Per @rousskov 12 Oct 2024: "Please do not block PRs just because you do not see value in them (when other core developers do see value)."

rousskov · 2024-11-21T20:44:24Z

Alex: Please do not block PRs just because you do not see value in them (when other core developers do see value).

I am not blocking this PR because I do not see its value. I am currently blocking this PR because its value is overwhelmed by its harm, while addressing my concern (and reversing that relationship) does not require heroic efforts.

This comment was marked as resolved.

Sign in to view

rousskov added the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label May 20, 2023

yadij removed the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label May 20, 2023

This comment was marked as resolved.

Sign in to view

yadij requested a review from rousskov May 20, 2023 23:16

yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 20, 2023

rousskov reviewed May 20, 2023

View reviewed changes

src/ip/QosConfig.cc Outdated Show resolved Hide resolved

rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 20, 2023

yadij added the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 21, 2023

yadij requested a review from rousskov May 21, 2023 01:03

rousskov removed their request for review May 22, 2023 01:32

rousskov removed the S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box label May 22, 2023

rousskov requested changes May 22, 2023

View reviewed changes

src/ip/QosConfig.cc Outdated Show resolved Hide resolved

src/ip/QosConfig.cc Outdated Show resolved Hide resolved

src/ip/QosConfig.cc Show resolved Hide resolved

src/ip/QosConfig.cc Outdated Show resolved Hide resolved

rousskov added the S-waiting-for-author author action is expected (and usually required) label May 22, 2023

yadij requested a review from rousskov May 22, 2023 16:33

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels May 22, 2023

rousskov requested changes May 22, 2023

View reviewed changes

src/ip/QosConfig.cc Outdated Show resolved Hide resolved

src/ip/QosConfig.cc Outdated Show resolved Hide resolved

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels May 22, 2023

yadij force-pushed the mingw-v7-9 branch from 9470fea to 9bec9f0 Compare May 24, 2023 17:02

yadij requested a review from rousskov May 24, 2023 17:07

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels May 24, 2023

yadij changed the title ~~MinGW-w64: fix TOS socket API~~ Portability cleanup setsockopt() calls May 24, 2023

rousskov requested changes May 24, 2023

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels May 24, 2023

Sakatok approved these changes May 26, 2023

View reviewed changes

yadij added the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label Jun 3, 2023

rousskov mentioned this pull request Jun 4, 2023

Make TCP SetSocketOption template public #1373

Draft

yadij removed the S-waiting-for-PR Closure of other PR(s), current or future, is expected (and usually required) label Jun 4, 2023

yadij requested a review from rousskov June 4, 2023 17:26

yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed S-waiting-for-author author action is expected (and usually required) labels Jun 4, 2023

rousskov requested changes Jun 4, 2023

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Jun 4, 2023

yadij force-pushed the mingw-v7-9 branch from 05ccd7d to 02b82a1 Compare June 23, 2023 14:37

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Aug 8, 2023

squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jan 9, 2024

kinkie added the M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels label Feb 14, 2024

yadij force-pushed the mingw-v7-9 branch from 02b82a1 to 213df95 Compare November 20, 2024 06:57

yadij requested a review from rousskov November 20, 2024 07:00

kinkie approved these changes Nov 20, 2024

View reviewed changes

rousskov requested changes Nov 20, 2024

View reviewed changes

rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Portability cleanup setsockopt() calls #1359

Portability cleanup setsockopt() calls #1359

yadij commented May 20, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

rousskov May 24, 2023

yadij Jun 4, 2023

rousskov Jun 4, 2023

rousskov Nov 20, 2024

yadij Nov 21, 2024

rousskov Nov 21, 2024

yadij commented Jun 3, 2023

yadij commented Jun 4, 2023

rousskov Jun 4, 2023

rousskov left a comment

yadij commented Nov 21, 2024

rousskov commented Nov 21, 2024

Portability cleanup setsockopt() calls #1359

Are you sure you want to change the base?

Portability cleanup setsockopt() calls #1359

Conversation

yadij commented May 20, 2023 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

rousskov May 24, 2023

Choose a reason for hiding this comment

yadij Jun 4, 2023

Choose a reason for hiding this comment

rousskov Jun 4, 2023

Choose a reason for hiding this comment

rousskov Nov 20, 2024

Choose a reason for hiding this comment

yadij Nov 21, 2024

Choose a reason for hiding this comment

rousskov Nov 21, 2024

Choose a reason for hiding this comment

yadij commented Jun 3, 2023

yadij commented Jun 4, 2023

rousskov Jun 4, 2023

Choose a reason for hiding this comment

rousskov left a comment

Choose a reason for hiding this comment

yadij commented Nov 21, 2024

rousskov commented Nov 21, 2024

yadij commented May 20, 2023 •

edited

Loading